Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc updates to tee up async ChannelMonitorUpdate persist for claims against closed channels #3413

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

#3355 did a lot of the most complex work towards being able to do async ChannelMonitorUpdate persistence for updates writing a preimage for a closed channel, and I'd intended to get the rest of it done in one PR. Sadly, things kept coming up, so there's a laundry list of small-ish changes which need to land first. This PR tees up those small changes (plus one relatively straightforward refactor that touches a lot of lines), with the final PR coming separately.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need to stare more at e9dbd83. Lot going on there, so any tips on reviewing it would be appreciated.

lightning/src/ln/reorg_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/reorg_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
Comment on lines 7652 to 7654
if remaining_in_flight != 0 {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be pulled out and done unconditionally prior to the channel assignment? We're checking it again later, which would be unnecessary if done earlier? IIUC, we'd only skip the logging below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I was trying to retain the log, which I think is pretty important. I cleaned the flow up and added more logging though.

Makes `test_durable_preimages_on_closed_channel` more robust
against changes to the order in which transactions are broadcast.
When deciding if we should remove a `PeerState` entry we want to
ensure we don't remove if there are pending updates in
`in_flight_monitor_updates`. Previously this was done with a simple
`in_flight_monitor_updates.is_empty()`, however this can prevent
removal of `PeerState` entries if a channel had an update at some
point (leaving an entry in the map) but the update was ultimately
completed.

Instead, we need to iterate over the entries in
`in_flight_monitor_updates` and decline to remove `PeerState`s only
if there is an entry for a pending update still in-flight.
On startup, if we have a channel which was closed immediately
before shutdown such that the `ChannelMonitorUpdate` marking the
channel as closed is still in-flight, it doesn't make sense to
generate a fresh `ChannelMonitorUpdate` marking the channel as
closed immediately after the existing in-flight one.

Here we detect this case and drop the extra update, though its not
all that harmful it does avoid some test changes in the coming
commits.
During block connection, we cannot apply `ChannelMonitorUpdate`s if
we're running during the startup sequence (i.e. before the user has
called any methods outside of block connection). We previously
handled this by simply always pushing any `ChannelMonitorUpdate`s
generated during block connection into the
`pending_background_events` queue.

However, this results in `ChannelMonitorUpdate`s going through the
queue when we could just push them immediately. Here we explicitly
check `background_events_processed_since_startup` and use that to
decide whether to push updates through the background queue
instead.
In the coming commits we'll start handling `ChannelMonitorUpdate`s
during channel closure in-line rather than after dropping locks via
`finish_close_channel`. In order to make that easy, here we add a
new `REMAIN_LOCKED_UPDATE_ACTIONS_PROCESSED_LATER` variant to
`handle_new_monitor_update!` which can attempt to apply an update
without dropping the locks and processing
`MonitorUpdateCompletionAction`s immediately.
Closing channels requires a two step process - first
`update_maps_on_chan_removal` is called while holding the same
per-peer lock under which the channel reached the terminal state,
then after dropping the same lock(s), `finish_close_channel` is
called.

Because the channel is closed and thus no further
`ChannelMonitorUpdate`s are generated for the off-chain state, we'd
previously applied the `ChannelMonitorUpdate` in
`finish_close_channel`. This was tweaked somewhat in
c99d3d7 when we stopped using
`u64::MAX` for any updates after closure. However, we worked around
the races that implied by setting the `update_id` only when we go
to apply the `ChannelMonitorUpdate`, rather than when we create it.

In a coming commit, we'll need to have an `update_id` immediately
upon creation (to track in-flight updates that haven't reached
application yet). This implies that we can no longer apply closure
`ChannelMonitorUpdate`s after dropping the per-peer lock(s), as the
updates must be well-ordered with any later updates to the same
channel, even after it has been closed.

Thus, here, we add `ChannelMonitorUpdate` handling to
`update_maps_on_chan_removal`, renaming it `locked_close_channel`
to better capture its new purpose.
@TheBlueMatt TheBlueMatt force-pushed the 2024-11-async-persist-claiming-from-closed-chan-1 branch from 7ec1631 to 0578332 Compare November 24, 2024 14:55
@TheBlueMatt
Copy link
Collaborator Author

Probably need to stare more at e9dbd83. Lot going on there, so any tips on reviewing it would be appreciated.

Its somewhat mechanical - basically just taking the ShutdownResult and passing it into the macro with locks held rather than letting it sit until finish_shutdown. The macro changes themselves are basically shifting the monitor update application to the macro, but leaving the post-apply updates for finish_shutdown (unlike the existing monitor update handling macro which does both, dropping the lock in the middle).

@TheBlueMatt
Copy link
Collaborator Author

Also rebased.

Copy link

codecov bot commented Nov 24, 2024

Codecov Report

Attention: Patch coverage is 89.58333% with 15 lines in your changes missing coverage. Please review.

Project coverage is 89.24%. Comparing base (2d6720e) to head (55b712e).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 88.61% 6 Missing and 8 partials ⚠️
lightning/src/chain/channelmonitor.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3413      +/-   ##
==========================================
- Coverage   89.24%   89.24%   -0.01%     
==========================================
  Files         130      130              
  Lines      106959   107009      +50     
  Branches   106959   107009      +50     
==========================================
+ Hits        95452    95496      +44     
+ Misses       8718     8713       -5     
- Partials     2789     2800      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

c99d3d7 updated
`ChannelMonitorUpdate::update_id` to continue counting up even
after the channel is closed. It, however, accidentally updated the
`ChannelMonitorUpdate` application logic to skip testing that
`ChannelMonitorUpdate`s are well-ordered after the channel has been
closed (in an attempt to ensure other checks in the same
conditional block were applied).

This fixes that oversight.
When we handle a `ChannelMonitorUpdate` completion we always
complete everything that was waiting on any updates to the same
channel all at once. Thus, we need to skip all updates if there's
pending updates besides the one that was just completed.

We handled this correctly for open channels, but the shortcut for
closed channels ignored any other pending updates entirely.

Here we fix this, which is ultimately required for tests which are
added in a few commits to pass.
@TheBlueMatt TheBlueMatt force-pushed the 2024-11-async-persist-claiming-from-closed-chan-1 branch from 0578332 to 55b712e Compare November 25, 2024 20:01
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a few occurrences on rg update_maps_on_chan_removal

BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
counterparty_node_id, funding_txo, update, channel_id,
});
if self.background_events_processed_since_startup.load(Ordering::Acquire) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment for this method currently sounds like we'll always push to the background events queue

Comment on lines +3154 to +3155
if updates.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID {
if self.latest_update_id + 1 != updates.update_id {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have test coverage for this fix?

Comment on lines +2952 to +2963
let in_flight_updates = $peer_state.in_flight_monitor_updates.entry(funding_txo)
.or_insert_with(Vec::new);
if !in_flight_updates.contains(&update) {
in_flight_updates.push(update.clone());
}
let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
counterparty_node_id,
funding_txo,
channel_id,
update,
};
$self.pending_background_events.lock().unwrap().push(event);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but I think it might be helpful to document why monitor updates are duplicated between in_flight_monitor_updates and pending_background_events. Intuitively it seems like storing them in pending_background_events would be sufficient.

if !in_flight_updates.contains(&update) {
in_flight_updates.push(update.clone());
}
let event = BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also check for duplicates before pushing this background event, like how we do for in_flight_updates just above?

@@ -3241,18 +3264,17 @@ macro_rules! handle_monitor_update_completion {
}

macro_rules! handle_new_monitor_update {
($self: ident, $update_res: expr, $chan: expr, _internal, $completed: expr) => { {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this macro is losing readability in this PR and the follow-up due to more cases being jammed into it. Thoughts on splitting this into a few different macros, like handle_monitor_update_res_internal for the first block, handle_initial_monitor_res for the INITIAL_MONITOR case, etc?

if self.background_events_processed_since_startup.load(Ordering::Acquire) {
// If a `ChannelMonitorUpdate` was applied (i.e. any time we have a funding txo and are
// not in the startup sequence, check if we need to handle any
// `ChannelUpdateCompletionAction`s.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/ChannelUpdateCompletionAction/MonitorUpdateCompletionAction

mem::drop(peer_state);
mem::drop(per_peer_state);

self.handle_monitor_update_completion_actions(update_actions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for !update_actions.is_empty() case.

Comment on lines +3997 to +3999
if let Some(peer_state_mtx) = per_peer_state.get(&shutdown_res.counterparty_node_id) {
let mut peer_state = peer_state_mtx.lock().unwrap();
if peer_state.in_flight_monitor_updates.get(&funding_txo).map(|l| l.is_empty()).unwrap_or(true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would make sense to put this handling in locked_close_channel so we don't need to acquire the lock again here? Probably doesn't matter too much though.

log_trace!(logger, "Channel is open and awaiting update, resuming it");
handle_monitor_update_completion!(self, peer_state_lock, peer_state, per_peer_state, chan);
} else {
log_trace!(logger, "Channel is open but not awaiting update");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this reachable? Seems like it might not be since we already checked that there are no-inflight updates. Missing test coverage if so

let mut shutdown_res = chan_phase_entry.get_mut().context_mut().force_shutdown(false, reason.clone());
let chan_phase = remove_channel_phase!(self, peer_state, chan_phase_entry, shutdown_res);
failed_channels.push(shutdown_res);
pending_msg_events.push(events::MessageSendEvent::HandleError {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the behavior changed a bit here in this commit, previously we would only push this message event if ChannelPhase::Funded. Just wanted to make sure this is intentional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants